⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

This comment was automatically generated by workflow using github-action-benchmark.

CC: @shaharsamocha7

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @Alon-Ti)

crates/prover/src/constraint_framework/expr.rs line 196 at r2 (raw file):

    }

    pub fn simplify(&self) -> Self {

Document
And put note that

Code quote:

 pub fn simplify(&self) -> Self {

crates/prover/src/constraint_framework/expr.rs line 391 at r2 (raw file):

    HashMap<String, BaseField>,
    HashMap<String, SecureField>,
);

move above ExprVariables

Code quote:

pub type ExprVarAssignment = (
    HashMap<(usize, usize, isize), BaseField>,
    HashMap<String, BaseField>,
    HashMap<String, SecureField>,
);

crates/prover/src/constraint_framework/expr.rs line 418 at r2 (raw file):

    }

    pub fn randomize(&self) -> ExprVarAssignment {

Document
and maybe rename to random_assignment? WDYT?

Code quote:

pub fn randomize(&self) -> ExprVarAssignment {

crates/prover/src/constraint_framework/expr.rs line 181 at r1 (raw file):

    }

    pub fn simplify(&self) -> Self {

rename to unsafe
and document that this shouldn't be used and refer to the simplify function

Code quote:

pub fn simplify(&self) -> Self {

crates/prover/src/constraint_framework/expr.rs line 248 at r1 (raw file):

    pub fn random_eval(&self) -> BaseField {
        let assignment = self.collect_variables().randomize();

Assert that assignment.2 is empty?

Code quote:

let assignment = self.collect_variables().randomize();

crates/prover/src/constraint_framework/expr.rs line 381 at r1 (raw file):

#[derive(Default)]
pub struct ExprVariables {

Document

Code quote:

pub struct ExprVariables {

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @shaharsamocha7)

crates/prover/src/constraint_framework/expr.rs line 181 at r1 (raw file):

Done.

crates/prover/src/constraint_framework/expr.rs line 248 at r1 (raw file):

Done.

crates/prover/src/constraint_framework/expr.rs line 381 at r1 (raw file):

Done.

crates/prover/src/constraint_framework/expr.rs line 196 at r2 (raw file):

Done.

crates/prover/src/constraint_framework/expr.rs line 391 at r2 (raw file):

Done.

crates/prover/src/constraint_framework/expr.rs line 418 at r2 (raw file):

Done.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alon-Ti)

crates/prover/src/constraint_framework/expr.rs line 418 at r2 (raw file):

Also write that this function uses only for tests.
Could we define it with cfg(tests) (not sure If I already suggested this)

crates/prover/src/constraint_framework/expr.rs line 1043 at r3 (raw file):

        let full_eval = expr.eval_expr::<AssertEvaluator<'_>, _, _, _>(&columns, &vars, &ext_vars);
        let simplified_eval = expr
            .unchecked_simplify()

why not just call simplify here?

Code quote:

unchecked_simplify

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alon-Ti)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)

crates/prover/src/constraint_framework/expr.rs line 418 at r2 (raw file):

It's not just for tests though, it should run every time an expression is simplified, including (especially) real use cases.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @Alon-Ti)